test(trade): seed one stack per deposit so every transfer is a pick-up and place - #4030
Merged
Conversation
u9g
force-pushed
the
test/trade-one-stack-per-deposit
branch
from
August 30, 2026 18:02
8f0c869 to
dd9b75c
Compare
u9g
force-pushed
the
test/trade-one-stack-per-deposit
branch
2 times, most recently
from
September 4, 2026 21:11
95b80c6 to
5a6f188
Compare
Contributor
|
Tests more than 1.5x slower than master (durations are noisy, so this is informational): |
Member
|
Approved, resolved conflict and merge |
…p and place transfer takes the first stack of the item it finds, so with two 64-stacks of emeralds each deposit picks up a stack, right-clicks the price in one at a time and puts the rest back: 36 right clicks for the sword trade. Seeding a stack of exactly the price for each deposit, in the order the trades consume them, makes every deposit two clicks: 101 -> 34 clicks before 1.14. On 1.14+ the server moves the stacks itself (29 -> 21). The slots are set concurrently: on 1.21.3+ there is no creative slot ack and each set waits 400ms for a rejection, which serialised would cost 4s.
u9g
force-pushed
the
test/trade-one-stack-per-deposit
branch
from
September 6, 2026 13:01
5a6f188 to
4556967
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebased on master. The first commit is the test half of #3983 (
trades = 2): with master's 11 uses, one stack per deposit would need 44 stacks, more than the inventory holds. If #3983 merges first this rebases down to the second commit.Problem
tradeseeds two 64-stacks of emeralds.bot.transfertakes the first stack of the item it finds, so each deposit picks up 64, right-clicks the price in one at a time and puts the rest back — 36 right clicks per deposit for the wooden sword trade. Every click is a server tick, so pre-1.14 the test spends ~5s in 101 clicks.Changes
Seed one stack of exactly the price for each deposit, in the order the trades consume them (
[p0, p0, 2, 2, 1, 1, 36, 36]emeralds and[1, 1]books).transferthen finds the exact stack first and every deposit is pick-up + place. Assertions are unchanged; they only check totals.The slots are set with
Promise.all(asclearInventoryalready does): on 1.21.3+ (noAckOnCreateSetSlotPacket) there is no ack forset_creative_slot, so eachsetInventorySlotwaits 400ms for a rejection; serialised over 10 slots that is 4s.Verification
tradeon the base branch vs this PR (clicks arewindow_clicks from the packet trace):On 1.14+ the server moves whole stacks into the trade slots itself when a trade is selected, so the click count barely changes there; the gain is from the concurrent slot setup. The remaining ~1s of the pre-1.14 runs is the two command-block sleeps (#4029).